[HVM][VMX] Clean up and audit VMX uses of instruction-length

info field. Todo: use by mmio decoder needs to be removed.
Signed-off-by: Keir Fraser <keir@xensource.com>

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 6a06173962e31d52e0903af17c914edc66007f0f..02e3316dcfa8cb996cd584702033133f3ca44dc7 100644 (file)
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1847,12 +1847,12 @@ static int svm_cr_access(struct vcpu *v, unsigned int cr, unsigned int type,
        where the prefix lives later on */
     index = skip_prefix_bytes(buffer, sizeof(buffer));
     
-    if (type == TYPE_MOV_TO_CR) 
+    if ( type == TYPE_MOV_TO_CR )
     {
         inst_len = __get_instruction_length_from_list(
             vmcb, list_a, ARR_SIZE(list_a), &buffer[index], &match);
     }
-    else
+    else /* type == TYPE_MOV_FROM_CR */
     {
         inst_len = __get_instruction_length_from_list(
             vmcb, list_b, ARR_SIZE(list_b), &buffer[index], &match);

diff --git a/xen/arch/x86/hvm/vmx/io.c b/xen/arch/x86/hvm/vmx/io.c
index f5133b3ce27a4afffa463937d35563654652da63..75632a41d6b984e78995d804de4bfc2a587664db 100644 (file)
--- a/xen/arch/x86/hvm/vmx/io.c
+++ b/xen/arch/x86/hvm/vmx/io.c
@@ -108,11 +108,17 @@ asmlinkage void vmx_intr_assist(void)
         return;
     }
 
+    /* This could be moved earlier in the VMX resume sequence. */
     __vmread(IDT_VECTORING_INFO_FIELD, &idtv_info_field);
     if (unlikely(idtv_info_field & INTR_INFO_VALID_MASK)) {
         __vmwrite(VM_ENTRY_INTR_INFO_FIELD, idtv_info_field);
 
-        __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
+        /*
+         * Safe: the length will only be interpreted for software exceptions
+         * and interrupts. If we get here then delivery of some event caused a
+         * fault, and this always results in defined VM_EXIT_INSTRUCTION_LEN.
+         */
+        __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len); /* Safe */
         __vmwrite(VM_ENTRY_INSTRUCTION_LEN, inst_len);
 
         if (unlikely(idtv_info_field & 0x800)) { /* valid error code */

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index a1e562e929f8f6072f1566ececced314af26be76..a889aaff05fcd9f5c9b41bc9bff788d9536f764d 100644 (file)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -597,7 +597,7 @@ static int vmx_instruction_length(struct vcpu *v)
 {
     unsigned long inst_len;
 
-    if (__vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len))
+    if ( __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len) ) /* XXX Unsafe XXX */
         return 0;
     return inst_len;
 }
@@ -836,11 +836,16 @@ int start_vmx(void)
 
 /*
  * Not all cases receive valid value in the VM-exit instruction length field.
+ * Callers must know what they're doing!
  */
-#define __get_instruction_length(len) \
-    __vmread(VM_EXIT_INSTRUCTION_LEN, &(len)); \
-     if ((len) < 1 || (len) > 15) \
-        __hvm_bug(&regs);
+static int __get_instruction_length(void)
+{
+    int len;
+    __vmread(VM_EXIT_INSTRUCTION_LEN, &len); /* Safe: callers audited */
+    if ( (len < 1) || (len > 15) )
+        __hvm_bug(guest_cpu_user_regs());
+    return len;
+}
 
 static void inline __update_guest_eip(unsigned long inst_len)
 {
@@ -1051,15 +1056,18 @@ static int check_for_null_selector(unsigned long eip)
     int i, inst_len;
     int inst_copy_from_guest(unsigned char *, unsigned long, int);
 
-    __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
+    inst_len = __get_instruction_length(); /* Safe: INS/OUTS */
     memset(inst, 0, MAX_INST_LEN);
-    if (inst_copy_from_guest(inst, eip, inst_len) != inst_len) {
+    if ( inst_copy_from_guest(inst, eip, inst_len) != inst_len )
+    {
         printf("check_for_null_selector: get guest instruction failed\n");
         domain_crash_synchronous();
     }
 
-    for (i = 0; i < inst_len; i++) {
-        switch (inst[i]) {
+    for ( i = 0; i < inst_len; i++ )
+    {
+        switch ( inst[i] )
+        {
         case 0xf3: /* REPZ */
         case 0xf2: /* REPNZ */
         case 0xf0: /* LOCK */
@@ -1184,15 +1192,14 @@ static void vmx_io_instruction(unsigned long exit_qualification,
     }
 }
 
-int
-vmx_world_save(struct vcpu *v, struct vmx_assist_context *c)
+static int vmx_world_save(struct vcpu *v, struct vmx_assist_context *c)
 {
-    unsigned long inst_len;
     int error = 0;
 
-    error |= __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
+    /* NB. Skip transition instruction. */
     error |= __vmread(GUEST_RIP, &c->eip);
-    c->eip += inst_len; /* skip transition instruction */
+    c->eip += __get_instruction_length(); /* Safe: MOV Cn, LMSW, CLTS */
+
     error |= __vmread(GUEST_RSP, &c->esp);
     error |= __vmread(GUEST_RFLAGS, &c->eflags);
 
@@ -1249,8 +1256,7 @@ vmx_world_save(struct vcpu *v, struct vmx_assist_context *c)
     return !error;
 }
 
-int
-vmx_world_restore(struct vcpu *v, struct vmx_assist_context *c)
+static int vmx_world_restore(struct vcpu *v, struct vmx_assist_context *c)
 {
     unsigned long mfn, old_cr4, old_base_mfn;
     int error = 0;
@@ -1364,8 +1370,7 @@ vmx_world_restore(struct vcpu *v, struct vmx_assist_context *c)
 
 enum { VMX_ASSIST_INVOKE = 0, VMX_ASSIST_RESTORE };
 
-int
-vmx_assist(struct vcpu *v, int mode)
+static int vmx_assist(struct vcpu *v, int mode)
 {
     struct vmx_assist_context c;
     u32 magic;
@@ -1408,8 +1413,8 @@ vmx_assist(struct vcpu *v, int mode)
         break;
 
         /*
-         * Restore the VMXASSIST_OLD_CONTEXT that was saved by VMX_ASSIST_INVOKE
-         * above.
+         * Restore the VMXASSIST_OLD_CONTEXT that was saved by
+         * VMX_ASSIST_INVOKE above.
          */
     case VMX_ASSIST_RESTORE:
         /* save the old context */
@@ -1552,7 +1557,8 @@ static int vmx_set_cr0(unsigned long value)
             }
         }
 
-        if ( vmx_assist(v, VMX_ASSIST_INVOKE) ) {
+        if ( vmx_assist(v, VMX_ASSIST_INVOKE) )
+        {
             set_bit(VMX_CPU_STATE_ASSIST_ENABLED, &v->arch.hvm_vmx.cpu_state);
             __vmread(GUEST_RIP, &eip);
             HVM_DBG_LOG(DBG_LEVEL_1,
@@ -1815,7 +1821,8 @@ static void mov_from_cr(int cr, int gp, struct cpu_user_regs *regs)
     HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR%d, value = %lx", cr, value);
 }
 
-static int vmx_cr_access(unsigned long exit_qualification, struct cpu_user_regs *regs)
+static int vmx_cr_access(unsigned long exit_qualification,
+                         struct cpu_user_regs *regs)
 {
     unsigned int gp, cr;
     unsigned long value;
@@ -2069,6 +2076,47 @@ void restore_cpu_user_regs(struct cpu_user_regs *regs)
 }
 #endif
 
+static void vmx_reflect_exception(struct vcpu *v)
+{
+    int error_code, intr_info, vector;
+
+    __vmread(VM_EXIT_INTR_INFO, &intr_info);
+    vector = intr_info & 0xff;
+    if ( intr_info & INTR_INFO_DELIVER_CODE_MASK )
+        __vmread(VM_EXIT_INTR_ERROR_CODE, &error_code);
+    else
+        error_code = VMX_DELIVER_NO_ERROR_CODE;
+
+#ifndef NDEBUG
+    {
+        unsigned long rip;
+
+        __vmread(GUEST_RIP, &rip);
+        HVM_DBG_LOG(DBG_LEVEL_1, "rip = %lx, error_code = %x",
+                    rip, error_code);
+    }
+#endif /* NDEBUG */
+
+    /*
+     * According to Intel Virtualization Technology Specification for
+     * the IA-32 Intel Architecture (C97063-002 April 2005), section
+     * 2.8.3, SW_EXCEPTION should be used for #BP and #OV, and
+     * HW_EXCEPTION used for everything else.  The main difference
+     * appears to be that for SW_EXCEPTION, the EIP/RIP is incremented
+     * by VM_ENTER_INSTRUCTION_LEN bytes, whereas for HW_EXCEPTION,
+     * it is not.
+     */
+    if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) == INTR_TYPE_SW_EXCEPTION )
+    {
+        int ilen = __get_instruction_length(); /* Safe: software exception */
+        vmx_inject_sw_exception(v, vector, ilen);
+    }
+    else
+    {
+        vmx_inject_hw_exception(v, vector, error_code);
+    }
+}
+
 asmlinkage void vmx_vmexit_handler(struct cpu_user_regs regs)
 {
     unsigned int exit_reason;
@@ -2116,7 +2164,8 @@ asmlinkage void vmx_vmexit_handler(struct cpu_user_regs regs)
 
     TRACE_VMEXIT(0,exit_reason);
 
-    switch ( exit_reason ) {
+    switch ( exit_reason )
+    {
     case EXIT_REASON_EXCEPTION_NMI:
     {
         /*
@@ -2242,43 +2291,38 @@ asmlinkage void vmx_vmexit_handler(struct cpu_user_regs regs)
         domain_crash_synchronous();
         break;
     case EXIT_REASON_CPUID:
-        vmx_vmexit_do_cpuid(&regs);
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: CPUID */
         __update_guest_eip(inst_len);
+        vmx_vmexit_do_cpuid(&regs);
         break;
     case EXIT_REASON_HLT:
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: HLT */
         __update_guest_eip(inst_len);
         vmx_vmexit_do_hlt();
         break;
     case EXIT_REASON_INVLPG:
     {
-        unsigned long   va;
-
+        unsigned long va;
+        inst_len = __get_instruction_length(); /* Safe: INVLPG */
+        __update_guest_eip(inst_len);
         __vmread(EXIT_QUALIFICATION, &va);
         vmx_vmexit_do_invlpg(va);
-        __get_instruction_length(inst_len);
-        __update_guest_eip(inst_len);
         break;
     }
     case EXIT_REASON_VMCALL:
     {
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: VMCALL */
+        __update_guest_eip(inst_len);
         __vmread(GUEST_RIP, &rip);
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-
         hvm_do_hypercall(&regs);
-        __update_guest_eip(inst_len);
         break;
     }
     case EXIT_REASON_CR_ACCESS:
     {
         __vmread(GUEST_RIP, &rip);
-        __get_instruction_length(inst_len);
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-
-        HVM_DBG_LOG(DBG_LEVEL_1, "rip = %lx, inst_len =%lx, exit_qualification = %lx",
-                    rip, inst_len, exit_qualification);
+        inst_len = __get_instruction_length(); /* Safe: MOV Cn, LMSW, CLTS */
         if ( vmx_cr_access(exit_qualification, &regs) )
             __update_guest_eip(inst_len);
         TRACE_VMEXIT(3,regs.error_code);
@@ -2291,19 +2335,19 @@ asmlinkage void vmx_vmexit_handler(struct cpu_user_regs regs)
         break;
     case EXIT_REASON_IO_INSTRUCTION:
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: IN, INS, OUT, OUTS */
         vmx_io_instruction(exit_qualification, inst_len);
         TRACE_VMEXIT(4,exit_qualification);
         break;
     case EXIT_REASON_MSR_READ:
-        __get_instruction_length(inst_len);
-        vmx_do_msr_read(&regs);
+        inst_len = __get_instruction_length(); /* Safe: RDMSR */
         __update_guest_eip(inst_len);
+        vmx_do_msr_read(&regs);
         break;
     case EXIT_REASON_MSR_WRITE:
-        vmx_do_msr_write(&regs);
-        __get_instruction_length(inst_len);
+        inst_len = __get_instruction_length(); /* Safe: WRMSR */
         __update_guest_eip(inst_len);
+        vmx_do_msr_write(&regs);
         break;
     case EXIT_REASON_MWAIT_INSTRUCTION:
     case EXIT_REASON_MONITOR_INSTRUCTION:

diff --git a/xen/include/asm-x86/hvm/svm/emulate.h b/xen/include/asm-x86/hvm/svm/emulate.h
index 7f693ebb23b1b803973f70f41f32eb63c0f59593..4d15652afee8125b78531051d7cba1415abbf05a 100644 (file)
--- a/xen/include/asm-x86/hvm/svm/emulate.h
+++ b/xen/include/asm-x86/hvm/svm/emulate.h
@@ -94,15 +94,14 @@ extern int __get_instruction_length_from_list(struct vmcb_struct *vmcb,
 static inline int __get_instruction_length(struct vmcb_struct *vmcb, 
         enum instruction_index instr, u8 *guest_eip_buf)
 {
-    return __get_instruction_length_from_list(vmcb, &instr, 1, guest_eip_buf, 
-            NULL);
+    return __get_instruction_length_from_list(
+        vmcb, &instr, 1, guest_eip_buf, NULL);
 }
 
 
 static inline unsigned int is_prefix(u8 opc)
 {
-    switch(opc)
-    {
+    switch ( opc ) {
     case 0x66:
     case 0x67:
     case 0x2E:
@@ -115,22 +114,7 @@ static inline unsigned int is_prefix(u8 opc)
     case 0xF3:
     case 0xF2:
 #if __x86_64__
-    case 0x40:
-    case 0x41:
-    case 0x42:
-    case 0x43:
-    case 0x44:
-    case 0x45:
-    case 0x46:
-    case 0x47:
-    case 0x48:
-    case 0x49:
-    case 0x4a:
-    case 0x4b:
-    case 0x4c:
-    case 0x4d:
-    case 0x4e:
-    case 0x4f:
+    case 0x40 ... 0x4f:
 #endif /* __x86_64__ */
         return 1;
     }
@@ -141,15 +125,15 @@ static inline unsigned int is_prefix(u8 opc)
 static inline int skip_prefix_bytes(u8 *buf, size_t size)
 {
     int index;
-    for (index = 0; index < size && is_prefix(buf[index]); index ++)  
-        /* do nothing */ ;
+    for ( index = 0; index < size && is_prefix(buf[index]); index++ )
+        continue;
     return index;
 }
 
 
 
-static void inline __update_guest_eip(struct vmcb_struct *vmcb, 
-        int inst_len) 
+static void inline __update_guest_eip(
+    struct vmcb_struct *vmcb, int inst_len) 
 {
     ASSERT(inst_len > 0);
     vmcb->rip += inst_len;

diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 38e447259c7e6c1924a3f7aad2d099ba2d3bc4b2..a60c4523af1dc3d71a61d9f2955e784ca3840121 100644 (file)
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -469,7 +469,7 @@ static inline void __vmx_inject_exception(struct vcpu *v, int trap, int type,
     if ( error_code != VMX_DELIVER_NO_ERROR_CODE ) {
         __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
         intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
-     }
+    }
 
     if ( ilen )
       __vmwrite(VM_ENTRY_INSTRUCTION_LEN, ilen);
@@ -499,40 +499,4 @@ static inline void vmx_inject_extint(struct vcpu *v, int trap, int error_code)
     __vmwrite(GUEST_INTERRUPTIBILITY_INFO, 0);
 }
 
-static inline void vmx_reflect_exception(struct vcpu *v)
-{
-    int error_code, intr_info, vector;
-
-    __vmread(VM_EXIT_INTR_INFO, &intr_info);
-    vector = intr_info & 0xff;
-    if ( intr_info & INTR_INFO_DELIVER_CODE_MASK )
-        __vmread(VM_EXIT_INTR_ERROR_CODE, &error_code);
-    else
-        error_code = VMX_DELIVER_NO_ERROR_CODE;
-
-#ifndef NDEBUG
-    {
-        unsigned long rip;
-
-        __vmread(GUEST_RIP, &rip);
-        HVM_DBG_LOG(DBG_LEVEL_1, "rip = %lx, error_code = %x",
-                    rip, error_code);
-    }
-#endif /* NDEBUG */
-
-    /* According to Intel Virtualization Technology Specification for
-       the IA-32 Intel Architecture (C97063-002 April 2005), section
-       2.8.3, SW_EXCEPTION should be used for #BP and #OV, and
-       HW_EXCPEPTION used for everything else.  The main difference
-       appears to be that for SW_EXCEPTION, the EIP/RIP is incremented
-       by VM_ENTER_INSTRUCTION_LEN bytes, whereas for HW_EXCEPTION,
-       it is not.  */
-    if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) == INTR_TYPE_SW_EXCEPTION ) {
-        int ilen;
-        __vmread(VM_EXIT_INSTRUCTION_LEN, &ilen);
-        vmx_inject_sw_exception(v, vector, ilen);
-    } else
-        vmx_inject_hw_exception(v, vector, error_code);
-}
-
 #endif /* __ASM_X86_HVM_VMX_VMX_H__ */